app/assets/stylesheets/embed.css.scss (1)

14-36: Extract the shared author-column width into an SCSS variable.

The 92px width appears in both .author (line 16) and .cooked margin-left (line 31). If one changes without the other, the layout breaks. A shared variable eliminates this risk.

♻️ Suggested refactor

+$author-column-width: 92px;
+
 article.post {
   border-bottom: 1px solid `#ddd`;

   .post-date {
     float: right;
     color: `#aaa`;
     font-size: 12px;
     margin: 4px 4px 0 0;
   }

   .author {
     padding: 20px 0;
-    width: 92px;
+    width: $author-column-width;
     float: left;

     text-align: center;

     h3 {
       text-align: center;
       color: `#4a6b82`;
       font-size: 13px;
       margin: 0;
     }
   }

   .cooked {
     padding: 20px 0;
-    margin-left: 92px;
+    margin-left: $author-column-width;

     p {
       margin: 0 0 1em 0;
     }
   }
 }

Gemfile (1)

209-211: Version pinning for new gems is optional and consistent with this Gemfile's pattern.

ruby-readability (0.7.3) and simple-rss (2.1.0) are both stable, well-maintained gems with millions of downloads—not abandonment-ware. While pinning versions is a best practice, most gems in this Gemfile don't have version constraints, so leaving these unpinned is consistent with the project's existing approach.

app/assets/javascripts/embed.js (1)

15-24: Origin check is loose — consider a stricter comparison.

discourseUrl.indexOf(e.origin) accepts any origin that is a substring of discourseUrl. While browser-set e.origin limits exploitation, a strict comparison against the parsed origin of discourseUrl would be more robust.

♻️ Suggested stricter origin check

  function postMessageReceived(e) {
    if (!e) { return; }
-   if (discourseUrl.indexOf(e.origin) === -1) { return; }
+   var a = document.createElement('a');
+   a.href = discourseUrl;
+   if (a.origin !== e.origin) { return; }

    if (e.data) {

db/migrate/20131217174004_create_topic_embeds.rb (1)

4-5: Consider adding an index on topic_id for efficient lookups.

Queries joining or filtering by topic_id (e.g., finding the embed record for a given topic) will benefit from an index. post_id may also warrant one depending on query patterns.

♻️ Add index

    add_index :topic_embeds, :embed_url, unique: true
+   add_index :topic_embeds, :topic_id

config/routes.rb (1)

245-246: Route addition looks correct.

The route maps cleanly to embed#best. Note: Brakeman flags that Rails 3.2.12 with globbing routes is vulnerable to directory traversal and RCE (CVE-2014-0130). While this specific route doesn't use globbing, upgrading Rails to at least 3.2.18 is strongly recommended to mitigate this framework-level vulnerability.

spec/jobs/poll_feed_spec.rb (2)

12-28: Negative tests may not isolate each condition independently.

Each test stubs only one setting but leaves the others unstubbed (relying on their defaults). If execute short-circuits on the first falsy condition, then e.g. the feed_polling_url test (line 18–22) might never actually check feed_polling_url because feed_polling_enabled? already defaults to false. Consider stubbing the other two settings to their "happy path" values in each negative test so that only the condition under test causes the early return.

Example for the `feed_polling_url` test

     it "requires feed_polling_url" do
+      SiteSetting.stubs(:feed_polling_enabled?).returns(true)
       SiteSetting.stubs(:feed_polling_url).returns(nil)
+      SiteSetting.stubs(:embed_by_username).returns(embed_by_username)
       poller.expects(:poll_feed).never
       poller.execute({})
     end

Apply the same pattern to the other two negative tests.

---

5-6: Extra empty lines flagged by RuboCop.

Static analysis flagged extra empty lines at block body beginning (line 5) and end (line 39). Remove them to satisfy Layout/EmptyLinesAroundBlockBody.

Proposed fix

 describe Jobs::PollFeed do
-
   let(:poller) { Jobs::PollFeed.new }

     end
   end
-
 end

Also applies to: 39-40

lib/post_creator.rb (1)

212-216: cook_method could be consolidated into the pass-through attributes list.

Line 216 duplicates the exact pattern used in the loop on lines 212–214. You could simply add :cook_method to the array to reduce repetition.

Proposed change

-    [:post_type, :no_bump, :cooking_options, :image_sizes, :acting_user, :invalidate_oneboxes].each do |a|
+    [:post_type, :no_bump, :cooking_options, :image_sizes, :acting_user, :invalidate_oneboxes, :cook_method].each do |a|
       post.send("#{a}=", `@opts`[a]) if `@opts`[a].present?
     end
-
-    post.cook_method = `@opts`[:cook_method] if `@opts`[:cook_method].present?

spec/components/topic_retriever_spec.rb (2)

21-25: Invalid-URL test doesn't stub embeddable_host — may pass for the wrong reason.

If TopicRetriever#retrieve checks embeddable_host before parsing the URL, this test passes because the host check fails (nil), not because the URL is invalid. Stub embeddable_host to a value so the URL-validation path is actually exercised.

Proposed fix

   it "does not call perform_retrieve when the embed url is not a url" do
+    SiteSetting.expects(:embeddable_host).returns("eviltrout.com")
     r = TopicRetriever.new("not a url")
     r.expects(:perform_retrieve).never
     r.retrieve
   end

---

5-6: Extra empty lines flagged by RuboCop.

Remove the extra blank lines at block body beginning (line 5) and end (lines 43, 45) per Layout/EmptyLinesAroundBlockBody.

Also applies to: 43-46

spec/controllers/embed_controller_spec.rb (2)

35-38: Using after for assertions is an anti-pattern.

Assertions in after blocks run even when the test body raises, producing confusing double-failures. Move these checks into each it block (or use a shared after only for teardown).

Proposed refactor — inline assertions

-      after do
-        response.should be_success
-        response.headers['X-Frame-Options'].should == "ALLOWALL"
-      end
-
       it "tells the topic retriever to work when no previous embed is found" do
         TopicEmbed.expects(:topic_id_for_embed).returns(nil)
         retriever = mock
         TopicRetriever.expects(:new).returns(retriever)
         retriever.expects(:retrieve)
         get :best, embed_url: embed_url
+        response.should be_success
+        response.headers['X-Frame-Options'].should == "ALLOWALL"
       end

       it "creates a topic view when a topic_id is found" do
         TopicEmbed.expects(:topic_id_for_embed).returns(123)
         TopicView.expects(:new).with(123, nil, {best: 5})
         get :best, embed_url: embed_url
+        response.should be_success
+        response.headers['X-Frame-Options'].should == "ALLOWALL"
       end

---

4-5: Extra empty lines flagged by RuboCop.

Lines 4, 30, 54, and 57 have extra blank lines per Layout/EmptyLinesAroundBlockBody. Remove them to satisfy the convention.

Also applies to: 54-58

spec/models/topic_embed_spec.rb (3)

34-34: start_with? assertion gives opaque failure messages.

If this assertion fails, RSpec will only report "expected true, got false" without showing the actual cooked value. Use should start_with(...) or should include(...) instead so failures display the actual vs. expected content.

Proposed fix

-        post.cooked.start_with?("hello world new post <a href=\"http://eviltrout.com/hello\">hello</a> <img src=\"http://eviltrout.com/images/wat.jpg\">").should be_true
+        post.cooked.should start_with("hello world new post <a href=\"http://eviltrout.com/hello\">hello</a> <img src=\"http://eviltrout.com/images/wat.jpg\">")

---

3-48: Missing test coverage for import_remote, topic_id_for_embed, and absolutize_urls.

The AI summary and other specs (embed_controller_spec, topic_retriever_spec) reference TopicEmbed.topic_id_for_embed and TopicEmbed.import_remote, but neither has a dedicated test here. absolutize_urls is only tested indirectly. Consider adding direct unit tests for these class methods, especially topic_id_for_embed which is used in the controller.

Would you like me to generate test stubs for these methods or open an issue to track this?

---

4-5: Extra empty lines flagged by RuboCop.

Multiple Layout/EmptyLinesAroundBlockBody violations at lines 4, 10, 12, 43, 45, and 47. Remove the extra blank lines.

Also applies to: 10-12, 43-48

app/jobs/regular/retrieve_topic.rb (1)

1-1: Unused require_dependency.

email/sender is never referenced in this file. This appears to be a copy-paste artifact.

Proposed fix

-require_dependency 'email/sender'
 require_dependency 'topic_retriever'

app/jobs/scheduled/poll_feed.rb (1)

20-22: feed_key is defined but never called — dead code.

This method computes a cache key but is not referenced anywhere in this file or, per the summary, elsewhere. If it's intended for future use (e.g., ETag/Last-Modified caching), consider removing it until then.

lib/topic_retriever.rb (1)

40-43: Synchronously executing the entire PollFeed job is expensive and fragile.

Jobs::PollFeed.new.execute({}) runs the full RSS poll inline—fetching and importing all feed items—just to check if one specific embed_url happens to be in the feed. Any error during the poll (network timeout, malformed feed) will propagate up and abort the single-topic retrieval. Consider either enqueuing PollFeed asynchronously or extracting a targeted lookup for the specific URL.

app/models/topic_embed.rb (1)

1-1: Use require instead of require_dependency for gems.

require_dependency is a Rails autoloading mechanism for application code. Gems like Nokogiri should be loaded with plain require.

Proposed fix

-require_dependency 'nokogiri'
+require 'nokogiri'